Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: eliminate fee double-charge by using configurable decorator #9051

Merged
merged 5 commits into from
Mar 11, 2024
Merged

fix: eliminate fee double-charge by using configurable decorator #9051

merged 5 commits into from
Mar 11, 2024

Conversation

JimLarson
Copy link
Contributor

@JimLarson JimLarson commented Mar 9, 2024 •
edited by michaelfig
Loading

refs: #9036

Description

The replacement for MempoolFeeDecorator suggested in the cosmos-sdk 0.46 upgrade guide does not behave the same as MempoolFeeDecorator and led to Txs being double-charged for fees.

The new Cosmos 0.46 DeductFeeDecorator subsumes the functionality of the old DeductFeeDecorator as well as the old MempoolFeeDecorator. We had created a local variation of the old DeductFeeDecorator with a configurable fee collector account.

Instead of cloning the old MempoolFeeDecorator (#9045), which increases our maintenance burden, we made the new DeductFeeDecorator configurable (agoric-labs/cosmos-sdk#407), so we'll use that instead.

Security Considerations

Usual scrutiny.

Scaling Considerations

N/A

Documentation Considerations

N/A

Testing Considerations

Testing for #9045 confirmed our understanding of the double-charge, but and further confirmation of the fix in an e2e test is desired available in agoric-sdk/a3p-integration/proposals/a:upgrade-next/ante-fees.test.js.

Upgrade Considerations

State-machine breaking, but otherwise N/A

@JimLarson JimLarson added bug Something isn't working agoric-cosmos labels Mar 9, 2024
@JimLarson JimLarson requested a review from gibson042 March 9, 2024 00:02
@JimLarson JimLarson self-assigned this Mar 9, 2024
@JimLarson
Copy link
Contributor Author

Note for reviewer - please confirm that cherry-pick of agoric-labs/cosmos-sdk#407 to Agoric-v0.46.16.2.x was done correctly.

@JimLarson JimLarson marked this pull request as ready for review March 9, 2024 00:32
gibson042
gibson042 approved these changes Mar 9, 2024
View reviewed changes
Copy link
Member

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, but I'm unclear on the agoric-labs/cosmos-sdk tag naming convention. I also think its CHANGELOG-Agoric.md could be a bit better:

 # Changelog (Agoric fork)
 
 ## Unreleased
 
-### Improvements
-
-* (auth) [#407](https://github.com/agoric-labs/cosmos-sdk/pull/407) Configurable fee collector module account in DeductFeeDecorator.
-
 ### API Breaking
 
 * (auth, bank) Agoric/agoric-sdk#8989 Remove deprecated lien support
 
 ## `v0.46.16-alpha.agoric.2.1` - 2024-03-08
 
 ### Improvements
 
-* (auth) #??? Configurable fee collector module account in DeductFeeDecorator. (backport #407)
+* (auth) [#407](https://github.com/agoric-labs/cosmos-sdk/pull/407) Configurable fee collector module account in DeductFeeDecorator.
 
 ## `v0.46.16-alpha.agoric.2` - 2024-02-08

golang/cosmos/go.mod
@@ -187,7 +187,7 @@ replace (
// Agoric-specific replacements:
replace (
// We need a fork of cosmos-sdk until all of the differences are merged.
github.com/cosmos/cosmos-sdk => github.com/agoric-labs/cosmos-sdk v0.46.16-alpha.agoric.2
github.com/cosmos/cosmos-sdk => github.com/agoric-labs/cosmos-sdk v0.46.16-alpha.agoric.2.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the new tag be v0.46.16-alpha.agoric.3? I feel like I'm missing something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.3 was already taken by future incompatible changes (waiting in the wings of an agoric-sdk PR, but I'm not sure which one). .2.1 implies the first published revision of .2, which is more accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use the current tip of Agoric since lien support was removed in agoric-labs/cosmos-sdk#385 (in accordance with #8988, which is on master but not cherry-picked into the upgrade-14 release branch). So we need to use a side branch of Agoric for the new fee decorator feature. Our naming convention is to use <cosmos-release>-alpha.agoric.N sequentially on Agoric, ...-alpha.agoric.N.M for sub-branches, and presumably ...N.M.P for sub-sub-branches, etc. I'll document the policy in that repo.

golang/cosmos/ante/ante.go Outdated
// subsumes former MempoolFeeDecorator
ante.NewDeductFeeDecoratorWithName(opts.AccountKeeper, opts.BankKeeper, opts.FeegrantKeeper, nil, opts.FeeCollectorName),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like commentary for pull requests rather than permanent baggage.

Suggested change
// subsumes former MempoolFeeDecorator
ante.NewDeductFeeDecoratorWithName(opts.AccountKeeper, opts.BankKeeper, opts.FeegrantKeeper, nil, opts.FeeCollectorName),
ante.NewDeductFeeDecoratorWithName(opts.AccountKeeper, opts.BankKeeper, opts.FeegrantKeeper, nil, opts.FeeCollectorName),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@gibson042 gibson042 added the force:integration Force integration tests to run on PR label Mar 9, 2024
gibson042
gibson042 approved these changes Mar 9, 2024
View reviewed changes
Copy link
Member

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few testing suggestions, but overall reaffirming approval.

a3p-integration/proposals/a:upgrade-next/ante-fees.test.js Show resolved Hide resolved
a3p-integration/proposals/a:upgrade-next/ante-fees.test.js Outdated Show resolved Hide resolved
a3p-integration/proposals/a:upgrade-next/ante-fees.test.js Show resolved Hide resolved
a3p-integration/proposals/a:upgrade-next/ante-fees.test.js Show resolved Hide resolved
a3p-integration/proposals/a:upgrade-next/ante-fees.test.js Outdated Show resolved Hide resolved
@JimLarson
Copy link
Contributor Author

@gibson042 Regarding CHANGELOG-Agoric.md in agoric-labs/cosmos-sdk, Agoric-v0.46.16.2.x branch - I messed up and forgot to create a working sub-branch and made the modifications in the above branch, and pushed it to origin, so there was no PR to mention. I tried to unwind it with a force-push, but the Agoric* prefix on the branch name prevented any modification except by PR. (Apparently the initial erroneous push was not subject to that check.) I considered pushing a PR to revert the modifications on the branch, then aother PR to redo the cherry-pick with the correct changelog entry - both of which would need review and approval - but that exceeded the monkey-business I was prepared to invest just to get the changelog perfect.

@michaelfig michaelfig force-pushed the 9036-antefix-cosmos branch from f6ba771 to 9ac6222 Compare March 11, 2024 00:41
@michaelfig michaelfig added the automerge:rebase Automatically rebase updates, then merge label Mar 11, 2024
@mergify mergify bot merged commit 177a8d0 into master Mar 11, 2024
70 of 77 checks passed
@mergify mergify bot deleted the 9036-antefix-cosmos branch March 11, 2024 01:14
This was referenced Mar 11, 2024
Closed
Closed
gibson042 pushed a commit that referenced this pull request Mar 11, 2024
@mergify @gibson042
Merge pull request #9051 from Agoric/9036-antefix-cosmos
5605892 
fix: eliminate fee double-charge by using configurable decorator
This was referenced Mar 11, 2024
Merged
Closed
gibson042 pushed a commit that referenced this pull request Mar 11, 2024
@mergify @gibson042
Merge pull request #9051 from Agoric/9036-antefix-cosmos
9c0255b 
fix: eliminate fee double-charge by using configurable decorator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelfig michaelfig michaelfig left review comments

@gibson042 gibson042 gibson042 approved these changes

Assignees

@JimLarson JimLarson

Labels
agoric-cosmos automerge:rebase Automatically rebase updates, then merge bug Something isn't working force:integration Force integration tests to run on PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JimLarson @michaelfig @gibson042